Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#5901] feat(core): support tag event to Gravitino server #5998

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

[#5901] feat(core): support tag event to Gravitino server #5998

wants to merge 13 commits into from

Conversation

TungYuChiang
Copy link
Contributor

@TungYuChiang TungYuChiang commented Dec 26, 2024 •
edited
Loading

What changes were proposed in this pull request?

  1. Create corresponding event like ListTagEvent
  2. Add corresponding event to TagEventDispatcher

Why are the changes needed?

Fix: #5901

Does this PR introduce any user-facing change?

no

How was this patch tested?

@orenccl orenccl requested a review from FANNG1 December 26, 2024 03:05
orenccl
orenccl suggested changes Dec 26, 2024
View reviewed changes
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java Outdated
@@ -72,7 +85,10 @@ public Tag getTag(String metalake, String name) throws NoSuchTagException {
// TODO: getTagPreEvent
try {
// TODO: getTagEvent
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant line?

core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java Outdated
@@ -121,7 +150,11 @@ public MetadataObject[] listMetadataObjectsForTag(String metalake, String name)
// TODO: listMetadataObjectsForTagPreEvent
try {
// TODO: listMetadataObjectsForTagEvent
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant line?

@orenccl
Copy link
Collaborator

orenccl commented Dec 26, 2024

It would be better to turn this PR into a draft before updating the unit tests.

@xunliu xunliu marked this pull request as draft December 26, 2024 08:26
@FANNG1
Copy link
Contributor

FANNG1 commented Dec 26, 2024

will review the PR until #5944 is merged for some duplicated logic, please wait for a while.

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 7, 2025

@TungYuChiang could you rebase your code since #5944 is merged

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 7, 2025

cc @cool9850311

@TungYuChiang
Copy link
Contributor Author

@TungYuChiang could you rebase your code since #5944 is merged

sure, I will complete it within the next few days.

@TungYuChiang TungYuChiang force-pushed the feature/tagEvent branch 4 times, most recently from a680fe6 to 43c2d20 Compare January 11, 2025 08:26
FANNG1
FANNG1 reviewed Jan 13, 2025
View reviewed changes
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java
// TODO: listTagsEvent
return dispatcher.listTags(metalake);
String[] tagNames = dispatcher.listTags(metalake);
eventBus.dispatchEvent(new ListTagEvent(PrincipalUtils.getCurrentUserName(), metalake));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename ListTagsFailureEvent to ListTagFailureEvent to keep consistent with other list event?

Copy link
Contributor Author

@TungYuChiang TungYuChiang Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename ListTagsFailureEvent to ListTagFailureEvent to keep consistent with other list event?

@FANNG1 , Would it be better to rename ListTagEvent to ListTagsEvent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ListTagsEvent is better

core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java
Tag[] tags = dispatcher.listTagsInfo(metalake);
eventBus.dispatchEvent(
new ListTagInfoEvent(PrincipalUtils.getCurrentUserName(), metalake, tags));
return tags;
} catch (Exception e) {
eventBus.dispatchEvent(
new ListTagsInfoFailureEvent(PrincipalUtils.getCurrentUserName(), metalake, e));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename ListTagsInfoFailureEvent to ListTagInfoFailureEvent to keep consistent with other list event?

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 13, 2025

Event represents an specific operation to specific resource, the current implement pass metalake as resource name, this's not correct for some event, please refer to XXTagFailureEvent for the definition of the resource.

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 13, 2025

please update the document too

@orenccl orenccl mentioned this pull request Jan 13, 2025
Open
@FANNG1
Copy link
Contributor

FANNG1 commented Jan 15, 2025

@TungYuChiang any progress? If this PR is merged these days, we could meet the 0.8 release.

@TungYuChiang
Copy link
Contributor Author

TungYuChiang commented Jan 15, 2025 •
edited
Loading

@TungYuChiang any progress? If this PR is merged these days, we could meet the 0.8 release.

@FANNG1 , I’m sorry for the delay. I’ve been a bit busy these past few days, but I’ll finish it by tomorrow evening.
Thank you for your patience

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 15, 2025

@TungYuChiang any progress? If this PR is merged these days, we could meet the 0.8 release.

@FANNG1 , I’m sorry for the delay. I’ve been a bit busy these past few days, but I’ll finish it by tomorrow evening. Thank you for your patience

no hurry, please take your time

@TungYuChiang
Add
eb810f3 
- [x]  GetTagForMetadataObject
- [x]  ListMetadataObjectsForTag
- [x]  AssociateTagsForMetadataObject
- [x]  ListTagsForMetadataObject
- [x]  ListTagsInfoForMetadataObject
@TungYuChiang
fix bugs
77aff4e
@TungYuChiang
Remove incorrect test
22722e2
@TungYuChiang
Copy link
Contributor Author

@FANNG1, I have fixed everything except renaming ListTagsInfoFailureEvent to ListTagInfoFailureEvent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FANNG1 FANNG1 FANNG1 left review comments

@orenccl orenccl orenccl requested changes

At least 1 approving review is required to merge this pull request.

Assignees

@TungYuChiang TungYuChiang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Support tag event to Gravitino server
3 participants
@TungYuChiang @orenccl @FANNG1